Support using GlobalObjectStore for non-field-injected contexts - #12381
Support using GlobalObjectStore for non-field-injected contexts#12381mcculls wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
286fa56 to
c7a0fb4
Compare
de73d43 to
ec23bdd
Compare
|
@DataDog review |
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies core runtime context storage and bytecode-injected fallback behavior, so it warrants a final human review to validate bootstrap/classloader safety and rollout risk.
Pull request overview
This PR adds a new configuration flag to control how non-field-injected InstrumentationContext storage behaves, allowing a rollout from the historical “weak map per store” fallback to the bounded, shared GlobalObjectStore implementation (from dd-instrument-java) for better scalability in scenarios like AOT class linking.
Changes:
- Introduces
trace.runtime.context.map-per-store(env:DD_TRACE_RUNTIME_CONTEXT_MAP_PER_STORE, defaulttrue) and wires it intoInstrumenterConfig. - Switches non-field-injected context storage in
FieldBackedContextStore(and injected accessor fallbacks) betweenWeakMapPerStoreandGlobalObjectStorebased on the new flag. - Refactors
ContextStorefactory APIs to usejava.util.function.Functionrather than a customKeyAwareFactory.
File summaries
| File | Description |
|---|---|
| metadata/supported-configurations.json | Registers the new env var flag in supported configuration metadata. |
| internal-api/src/main/java/datadog/trace/bootstrap/ContextStore.java | Replaces custom key-aware factory type with Function and adapts Factory accordingly. |
| internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java | Loads and exposes the new runtimeContextMapPerStore flag. |
| dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java | Adds the default value constant for the new flag. |
| dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java | Defines the canonical config key trace.runtime.context.map-per-store. |
| dd-java-agent/instrumentation/reactive-streams-1.0/src/test/java/datadog/trace/instrumentation/reactivestreams/ReactiveStreamsContextPropagationTest.java | Updates a test ContextStore implementation to the new Function-based API. |
| dd-java-agent/instrumentation/graal/graal-native-image-20.0/src/main/java/datadog/trace/instrumentation/graal/nativeimage/NativeImageGeneratorRunnerInstrumentation.java | Adds GlobalObjectStore classes to Graal native-image init-at-build-time list. |
| dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedContextInjector.java | Selects the injected fallback redirect class (WeakMapPerStore vs GlobalObjectStore) based on the new flag. |
| dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/utils/StrongMapContextStore.java | Updates the store implementation to accept Function factories. |
| dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapPerStore.java | Updates weak fallback store compute path to use Function.apply. |
| dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/FieldBackedContextStore.java | Switches non-field-injected storage between WeakMapPerStore and GlobalObjectStore under the feature flag. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
This change removes KeyAwareFactory and changes the ContextStore#getOrCompute method descriptor. Existing runtime extensions that use this API can fail to load.
🤖 Datadog Autotest · Commit ec23bdd · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec23bddc77
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
9369ab2 to
edd9ecf
Compare
What Does This Do
Adds a feature-flag to support switching non-field-injected
InstrumentationContextfrom using aWeakMapper-store to instead use the sharedGlobalObjectStorefrom dd-instrument-java.The default value of this feature-flag is
true, in other words the historical behaviour of using a map-per-store:To switch to
GlobalObjectStoreset the following environment variable:or add this JVM option:
Motivation
The
GlobalObjectStoreapproach scales better for deployments using a lot of non-field-injected instrumentation context where periodic cleanup / GC cannot keep up. For example on AOT where we cannot field-inject classes linked at training time. See #10479 for more details.GlobalObjectStoreuses a generational approach to evict old content, keeping the store bounded while still allowing tracking of recent instrumentation contexts. Whereas the map-per-store approach immediately drops new content if the store reaches capacity, and this causes async propagation issues on AOT until GC finally kicks in.Since the generational strategy is relatively new, the idea is to roll this out incrementally as we complete further benchmarking of various real-world applications.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]